[Driver] Reject unsupported -mcmodel= #70262
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
-mcmodel= is supported for a few architectures. Reject the option for other architectures. * -mcmodel= is unsupported on x86-32. * -mcmodel=large is unsupported for PIC on AArch64. * -mcmodel= is unsupported for aarch64_32 triples. * https://reviews.llvm.org/D67066 (for RISC-V) made -mcmodel=medany/-mcmodel=medlow aliases for all architectures. Restrict this to RISC-V. * llvm/lib/Target/Sparc has some small/medium/large support, but the values listed on https://gcc.gnu.org/onlinedocs/gcc/SPARC-Options.html had been supported before https://reviews.llvm.org/D67066. Consider -mcmodel= unsupported for Sparc. * https://reviews.llvm.org/D106371 translated -mcmodel=medium to -mcmodel=large on AIX, even for 32-bit systems. Retain this behavior but reject -mcmodel= for other PPC32 systems. In general the accept/reject behavior is more similar to GCC. err_drv_invalid_argument_to_option is less clear than err_drv_unsupported_option_argument. As the supported values are different for different architectures, add a err_drv_unsupported_option_argument_for_target for better clarity.
MaskRay
requested review from
smithp35,
kito-cheng,
hubert-reinterpretcast,
jrtc27 and
brad0
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Fangrui Song (MaskRay) Changes-mcmodel= is supported for a few architectures. Reject the option for
In general the accept/reject behavior is more similar to GCC. err_drv_invalid_argument_to_option is less clear than Full diff: https://github.com/llvm/llvm-project/pull/70262.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 5dbc5b5edfb4aeb..c0ccd64a2a7b82e 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -20,6 +20,8 @@ def err_drv_unsupported_opt_for_language_mode : Error<
"unsupported option '%0' for language mode '%1'">;
def err_drv_unsupported_option_argument : Error<
"unsupported argument '%1' to option '%0'">;
+def err_drv_unsupported_option_argument_for_target : Error<
+ "unsupported argument '%1' to option '%0' for target '%2'">;
def err_drv_unknown_stdin_type : Error<
"-E or -x required when input is from standard input">;
def err_drv_unknown_stdin_type_clang_cl : Error<
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index c6b1903a32a0621..93ddc452f61da88 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4505,14 +4505,6 @@ def msave_restore : Flag<["-"], "msave-restore">, Group<m_riscv_Features_Group>,
def mno_save_restore : Flag<["-"], "mno-save-restore">, Group<m_riscv_Features_Group>,
HelpText<"Disable using library calls for save and restore">;
} // let Flags = [TargetSpecific]
-def mcmodel_EQ_medlow : Flag<["-"], "mcmodel=medlow">, Group<m_Group>,
- Visibility<[ClangOption, CC1Option]>,
- Alias<mcmodel_EQ>, AliasArgs<["small"]>,
- HelpText<"Equivalent to -mcmodel=small, compatible with RISC-V gcc.">;
-def mcmodel_EQ_medany : Flag<["-"], "mcmodel=medany">, Group<m_Group>,
- Visibility<[ClangOption, CC1Option]>,
- Alias<mcmodel_EQ>, AliasArgs<["medium"]>,
- HelpText<"Equivalent to -mcmodel=medium, compatible with RISC-V gcc.">;
let Flags = [TargetSpecific] in {
def menable_experimental_extensions : Flag<["-"], "menable-experimental-extensions">, Group<m_Group>,
HelpText<"Enable use of experimental RISC-V extensions.">;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 601bbfb927746fc..43a92adbef64ba8 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5722,18 +5722,33 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
if (Arg *A = Args.getLastArg(options::OPT_mcmodel_EQ)) {
StringRef CM = A->getValue();
- if (CM == "small" || CM == "kernel" || CM == "medium" || CM == "large" ||
- CM == "tiny") {
- if (Triple.isOSAIX() && CM == "medium")
- CmdArgs.push_back("-mcmodel=large");
- else if (Triple.isAArch64() && (CM == "kernel" || CM == "medium"))
- D.Diag(diag::err_drv_invalid_argument_to_option)
- << CM << A->getOption().getName();
- else
- A->render(Args, CmdArgs);
+ bool Ok = false;
+ if (Triple.isOSAIX() && CM == "medium") {
+ CM = "large";
+ Ok = true;
+ }
+ if (Triple.isAArch64(64)) {
+ Ok = CM == "tiny" || CM == "small" || CM == "large";
+ if (CM == "large" && RelocationModel != llvm::Reloc::Static)
+ D.Diag(diag::err_drv_argument_only_allowed_with)
+ << A->getAsString(Args) << "-fno-pic";
+ } else if (Triple.isPPC64()) {
+ Ok = CM == "small" || CM == "medium" || CM == "large";
+ } else if (Triple.isRISCV()) {
+ if (CM == "medlow")
+ CM = "small";
+ else if (CM == "medany")
+ CM = "medium";
+ Ok = CM == "small" || CM == "medium";
+ } else if (Triple.getArch() == llvm::Triple::x86_64) {
+ Ok = llvm::is_contained({"small", "kernel", "medium", "large", "tiny"},
+ CM);
+ }
+ if (Ok) {
+ CmdArgs.push_back(Args.MakeArgString("-mcmodel=" + CM));
} else {
- D.Diag(diag::err_drv_invalid_argument_to_option)
- << CM << A->getOption().getName();
+ D.Diag(diag::err_drv_unsupported_option_argument_for_target)
+ << A->getSpelling() << CM << TripleStr;
}
}
diff --git a/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c b/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
index e758b384bc6eb23..9bd69e7995877e4 100644
--- a/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
+++ b/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
@@ -27,8 +27,6 @@
// RUN: | FileCheck %s -check-prefix=RV64-ANDROID
// RUN: %clang --target=riscv64-unknown-elf %s -S -emit-llvm -fpic -o - \
// RUN: | FileCheck %s -check-prefix=RV64-PIC
-// RUN: %clang --target=riscv64-unknown-elf %s -S -emit-llvm -mcmodel=large -o - \
-// RUN: | FileCheck %s -check-prefix=RV64-LARGE
void test(void) {}
diff --git a/clang/test/Driver/mcmodel.c b/clang/test/Driver/mcmodel.c
index 43aeb96af0bd0e1..fb3bbccb0c68157 100644
--- a/clang/test/Driver/mcmodel.c
+++ b/clang/test/Driver/mcmodel.c
@@ -1,14 +1,18 @@
+// RUN: not %clang -### -c --target=i686 -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=ERR-MEDIUM %s
// RUN: %clang --target=x86_64 -### -c -mcmodel=tiny %s 2>&1 | FileCheck --check-prefix=TINY %s
// RUN: %clang --target=x86_64 -### -c -mcmodel=small %s 2>&1 | FileCheck --check-prefix=SMALL %s
// RUN: %clang --target=x86_64 -### -S -mcmodel=kernel %s 2>&1 | FileCheck --check-prefix=KERNEL %s
// RUN: %clang --target=x86_64 -### -c -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=MEDIUM %s
// RUN: %clang --target=x86_64 -### -S -mcmodel=large %s 2>&1 | FileCheck --check-prefix=LARGE %s
+// RUN: not %clang -### -c --target=powerpc-linux-gnu -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=ERR-MEDIUM %s
// RUN: %clang --target=powerpc-unknown-aix -### -S -mcmodel=medium %s 2> %t.log
// RUN: FileCheck --check-prefix=AIX-MCMEDIUM-OVERRIDE %s < %t.log
// RUN: not %clang -### -c -mcmodel=lager %s 2>&1 | FileCheck --check-prefix=INVALID %s
// RUN: %clang --target=aarch64 -### -S -mcmodel=large -fno-pic %s 2>&1 | FileCheck --check-prefix=LARGE %s
+// RUN: not %clang --target=aarch64 -### -S -mcmodel=large -fpic %s 2>&1 | FileCheck --check-prefix=AARCH64-PIC-LARGE %s
// RUN: not %clang -### -c --target=aarch64 -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=ERR-MEDIUM %s
// RUN: not %clang -### -c --target=aarch64 -mcmodel=kernel %s 2>&1 | FileCheck --check-prefix=ERR-KERNEL %s
+// RUN: not %clang --target=aarch64_32-linux -### -S -mcmodel=small %s 2>&1 | FileCheck --check-prefix=ERR-AARCH64_32 %s
// TINY: "-mcmodel=tiny"
// SMALL: "-mcmodel=small"
@@ -17,7 +21,11 @@
// LARGE: "-mcmodel=large"
// AIX-MCMEDIUM-OVERRIDE: "-mcmodel=large"
-// INVALID: error: invalid argument 'lager' to -mcmodel=
+// INVALID: error: unsupported argument 'lager' to option '-mcmodel=' for target '{{.*}}'
-// ERR-MEDIUM: error: invalid argument 'medium' to -mcmodel=
-// ERR-KERNEL: error: invalid argument 'kernel' to -mcmodel=
+// ERR-MEDIUM: error: unsupported argument 'medium' to option '-mcmodel=' for target '{{.*}}'
+// ERR-KERNEL: error: unsupported argument 'kernel' to option '-mcmodel=' for target '{{.*}}'
+// ERR-LARGE: error: unsupported argument 'large' to option '-mcmodel=' for target '{{.*}}'
+
+// AARCH64-PIC-LARGE: error: invalid argument '-mcmodel=large' only allowed with '-fno-pic'
+// ERR-AARCH64_32: error: unsupported argument 'small' to option '-mcmodel=' for target 'aarch64_32-unknown-linux'
diff --git a/clang/test/Driver/riscv-sdata-warning.c b/clang/test/Driver/riscv-sdata-warning.c
index dbb5f5de9cd816c..ace9a32ee116fd8 100644
--- a/clang/test/Driver/riscv-sdata-warning.c
+++ b/clang/test/Driver/riscv-sdata-warning.c
@@ -2,11 +2,3 @@
// RUN: %clang -S --target=riscv32-unknown-elf -fpic -msmall-data-limit=8 %s 2>&1 \
// RUN: | FileCheck -check-prefix=CHECK-PIC-SDATA %s
// CHECK-PIC-SDATA: warning: ignoring '-msmall-data-limit=' with -mcmodel=large for -fpic or RV64
-
-// RUN: %clang -S --target=riscv64-unknown-elf -mcmodel=large -msmall-data-limit=8 %s 2>&1 \
-// RUN: | FileCheck -check-prefix=CHECK-RV64-LARGE-SDATA %s
-// CHECK-RV64-LARGE-SDATA: warning: ignoring '-msmall-data-limit=' with -mcmodel=large for -fpic or RV64
-
-// RUN: %clang -S --target=riscv64-linux-android -msmall-data-limit=8 %s 2>&1 \
-// RUN: | FileCheck -check-prefix=CHECK-RV64-LARGE-SDATA-ANDROID %s
-// CHECK-RV64-LARGE-SDATA-ANDROID: warning: ignoring '-msmall-data-limit=' with -mcmodel=large for -fpic or RV64
diff --git a/clang/test/Preprocessor/init-x86.c b/clang/test/Preprocessor/init-x86.c
index 58be9b716571747..1268419e18a5c4c 100644
--- a/clang/test/Preprocessor/init-x86.c
+++ b/clang/test/Preprocessor/init-x86.c
@@ -802,9 +802,6 @@
// X86_64H:#define __x86_64h 1
// X86_64H:#define __x86_64h__ 1
-// RUN: %clang -xc - -E -dM -mcmodel=medium --target=i386-unknown-linux < /dev/null | FileCheck -match-full-lines -check-prefix X86_MEDIUM %s
-// X86_MEDIUM:#define __code_model_medium__ 1
-
// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=x86_64-none-none-gnux32 < /dev/null | FileCheck -match-full-lines -check-prefix X32 %s
// RUN: %clang_cc1 -x c++ -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=x86_64-none-none-gnux32 < /dev/null | FileCheck -match-full-lines -check-prefix X32 -check-prefix X32-CXX %s
//
|
smithp35
reviewed
Oct 26, 2023
There was a problem hiding this comment.
I'm happy with this on the AArch64 side.
If clang for a target defaults to position independent code this may lead to some -mcmodel=large projects failing to build, at least until they add -fno-pic
Could we add this to the release notes? Perhaps in the form that users of -mcmodel=large on AArch64 are recommended to add -fno-pic.
I think this is better than -mcmodel=large silently producing non position independent code.
|
LGTM for the RISC-V bits, thanks :) |
brad0
approved these changes
Oct 26, 2023
|
This is not being handled for AMDGPU Targets. @arsenm @yxsamliu @kzhuravl @ronlieb Is following case correct for AMDGPU Target ? I am not sure about the previous state before this change for AMGPU Target. |
I'm assuming this is an artifact of passing all arguments both the host target and the offload target? @jhuber6 what's the correct way of filtering out irrelevant codegen options? |
I don't know what the desired behavior is. The |
|
see #70740 for ignoring this in NVPTX |
AMDGPU could be handled the same way |
MaskRay
added a commit
to MaskRay/llvm-project
that referenced
this pull request
Oct 31, 2023
There is no PIC support for -mcmodel=large (https://github.com/ARM-software/abi-aa/blob/main/sysvabi64/sysvabi64.rst) and Clang recently rejects -mcmodel= with PIC (llvm#70262). The current backend code assumes that the large code model is non-PIC. This patch adds `!getTargetMachine().isPositionIndependent()` conditions to clarify that the support is non-PIC only. In addition, add some tests as change detectors in case PIC large code model is supported in the future. If other front-ends/JITs use the large code model with PIC, they will get small code model code sequence, instead of potentially-incorrect MOVZ/MOVK sequence, which is only suitable for non-PIC. The sequence will cause text relocations using ELF linkers. (The small code model code sequence is usually sufficient as ADRP+ADD or ADRP+LDR targets [-2**32,2**32), which has a doubled range of x86-64 R_X86_64_REX_GOTPCRELX/R_X86_64_PC32 [-2**32,2**32).)
MaskRay
added a commit
that referenced
this pull request
Nov 1, 2023
There is no PIC support for -mcmodel=large (https://github.com/ARM-software/abi-aa/blob/main/sysvabi64/sysvabi64.rst) and Clang recently rejects -mcmodel= with PIC (#70262). The current backend code assumes that the large code model is non-PIC. This patch adds `!getTargetMachine().isPositionIndependent()` conditions to clarify that the support is non-PIC only. In addition, add some tests as change detectors in case PIC large code model is supported in the future. If other front-ends/JITs use the large code model with PIC, they will get small code model code sequence, instead of potentially-incorrect MOVZ/MOVK sequence, which is only suitable for non-PIC. The sequence will cause text relocations using ELF linkers. (The small code model code sequence is usually sufficient as ADRP+ADD or ADRP+LDR targets [-2**32,2**32), which has a doubled range of x86-64 R_X86_64_REX_GOTPCRELX/R_X86_64_PC32 [-2**32,2**32).)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
-mcmodel= is supported for a few architectures. Reject the option for
other architectures.
In general the accept/reject behavior is more similar to GCC.
err_drv_invalid_argument_to_option is less clear than
err_drv_unsupported_option_argument. As the supported values are different for
different architectures, add a err_drv_unsupported_option_argument_for_target
for better clarity.